Consider using #[cfg(any(test, feature = "testing"))] instead of just #[cfg(feature = "testing")].

The current approach means that unit tests in the core crate itself cannot use all_decorators() when running cargo test without explicitly enabling the testing feature.

The suggested alternative is the standard pattern for test helpers that need to be shared across crates while still being available for local unit tests.

With the 1-1 mapping now in place, could we simplify this function? The current implementation builds a BTreeMap and does two passes, but since every operation is now directly linked to its AsmOp, we can query by index directly.

Something like:

pub fn get_assembly_op(
    &self,
    node_id: MastNodeId,
    target_op_idx: Option<usize>,
) -> Option<&AssemblyOp> {
    let node = &self[node_id];

    let find_asmop = |ids: &[DecoratorId]| {
        ids.iter().find_map(|&id| match &self[id] {
            Decorator::AsmOp(asm_op) => Some(asm_op),
            _ => None,
        })
    };

    match target_op_idx {
        Some(idx) => match node {
            MastNode::Block(_) => find_asmop(self.decorators_for_operation(node_id, idx)),
            _ => match idx {
                0 => find_asmop(self.before_enter_decorators(node_id)),
                1 => find_asmop(self.after_exit_decorators(node_id)),
                _ => None,
            },
        },

        None => {
            // Search order: before_enter, then per-op (for blocks), then after_exit
            find_asmop(self.before_enter_decorators(node_id))
                .or_else(|| match node {
                    MastNode::Block(block) => (0..block.num_operations() as usize)
                        .find_map(|i| find_asmop(self.decorators_for_operation(node_id, i))),
                    _ => None,
                })
                .or_else(|| find_asmop(self.after_exit_decorators(node_id)))
        }
    }
}

This removes the Box<dyn Iterator> and BTreeMap, and the common case (Block + target index) becomes a direct lookup.

I'm still at a loss as to why we're doing this in the first place. Bobbin's answer to this question was that it would simplify the decorator finding logic - but comparing this with what we currently have, I find them pretty equally complex. IMO, most of the complexity comes from the fact that we have "before enter", "after exit", and sometimes "operations" decorators. The num_cycles check is not really the issue - it only applies to "operations" decorators, and is honestly pretty simple.

In contrast, the clear downside of this new approach is increased memory usage.

Am I missing something?

Perhaps it was worth it as an experiment which has shown it may be best to not push this one further? WDYT @bobbinth ?
Though I would add that the above is a bit complex to avoid allocations, and non-allocative code is always a bit involved.